fix: 2FA bug fixes #19260

BeeMargarida · 2023-05-19T11:32:23Z

Details

Fix several small issues related with 2FA.

Missing Download translation in Spanish - https://expensify.slack.com/archives/C049HHMV9SM/p1684406965776349
Two-factor name is inconsistent https://expensify.slack.com/archives/C049HHMV9SM/p1684406403576119
~~Offline Indicator displayed twice https://expensify.slack.com/archives/C049HHMV9SM/p1684408727072029~~
- This will be fixed in another issue, since it's present in other places - [HOLD for payment 2023-06-12] [Tracking Issue]Bug fixes: 2FA #19187 (comment)
Here is your magic code page appears https://expensify.slack.com/archives/C049HHMV9SM/p1684405641601719
Error message persists https://expensify.slack.com/archives/C049HHMV9SM/
View disrupted on small screen devices https://expensify.slack.com/archives/C049HHMV9SM/p1684392775324939

Fixed Issues

$ #19187
#19187 (comment)

Tests

Problem 1:

Sign in and enter the app
Change language to Spanish
Go to Seguridad -> Autenticación de dos factores
See that the button says 'Descargar' instead of Download

Problem 2:

Navigate to Sign in page
Enter email & click on continue button
Open email, and copy magic sign in link
Paste magic sign-in link in a new tab & press enter
Observe that it says 'Two-factor authentication required' instead of 'Two factor authentication required'

Problem 4:

Go to Sign in page and enter the email of an account with 2FA enabled and click next
Open received email, and copy magic sign in link (check that this is using staging URL, if not change it)
Paste this link in a new tab & press enter
Observe that 'Two-factor authentication required' page is displayed
Go to the tab where you were signing in, which should have moved on to 2FA code
Enter the 2FA code
Verify that user is signed in
Shift to the tab where 'Two-factor authentication required' page is displayed
Observe that it should now show 'Abracadabra, you are signed in!'

Problem 5:

Go to the Settings > Security > Two-factor authentication
Copy to clipboard and click "Next"
Enter an invalid 6 digit number and notice an error message shows up
Click the cancel icon and close the side panel
Click on settings > security > 2FA again
Notice a new code appears
Click on copy and click next
Observe that the old error message does not appear

Problem 6:

In a small screen device (Samsung Galaxy S5 - available in browser dev tools - or in a device with 720 x 1640 Pixels (~264 PPI)) open the app/browser page
Go to the Settings > Security > Two-factor authentication
Verify that the codes appear in two columns and do not overlap the buttons

Verify that no errors appear in the JS console

Offline tests

Do not apply for these specific issues, the only one that should affect won't be solved in this PR.

QA Steps

Problem 1:

Sign in and enter the app
Change language to Spanish
Go to Seguridad -> Autenticación de dos factores
See that the button says 'Descargar' instead of Download

Problem 2:

Navigate to Sign in page
Enter email & click on continue button
Open email, and copy magic sign in link
Paste magic sign-in link in a new tab & press enter
Observe that it says 'Two-factor authentication required' instead of 'Two factor authentication required'

Problem 4:

Go to Sign in page and enter the email of an account with 2FA enabled and click next
Open received email, and copy magic sign in link (locally this link needs to be switched to use localhost)
Paste this link in a new tab & press enter
Observe that 'Two-factor authentication required' page is displayed
Go to the tab where you were signing in, which should have moved on to 2FA code
Enter the 2FA code
Verify that user is signed in
Shift to the tab where 'Two-factor authentication required' page is displayed
Observe that it should now show 'Abracadabra, you are signed in!'

Problem 5:

Go to the Settings > Security > Two-factor authentication
Copy to clipboard and click "Next"
Enter an invalid 6 digit number and notice an error message shows up
Click the cancel icon and close the side panel
Click on settings > security > 2FA again
Notice a new code appears
Click on copy and click next
Observe that the old error message does not appear

Problem 6:

In a small screen device (Samsung Galaxy S5 - available in browser dev tools - or in a device with 720 x 1640 Pixels (~264 PPI)) open the app/browser page
Go to the Settings > Security > Two-factor authentication
Verify that the codes appear in two columns and do not overlap the buttons

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Problem 1:

Problem 2:

Problem 4:

web.mp4

Problem 5 and 6:

web1.mp4

Mobile Web - Chrome

Problem 6:

Mobile Web - Safari

Problem 6:

Desktop

iOS

Problem 6:

Android

Problem 6:

src/pages/ValidateLoginPage/index.website.js

src/pages/settings/Security/TwoFactorAuth/CodesPage.js

src/styles/utilities/spacing.js

melvin-bot · 2023-05-19T13:23:22Z

@MonilBhavsar @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

MonilBhavsar

Looks good to me 👍 Thanks!
@mananjadhav would like to get your review here and finish the checklist

mananjadhav

Code changes are fine, I'll test on all platforms and update the checklist.

mananjadhav · 2023-05-26T06:02:09Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-2fa-error-persists.mov

Mobile Web - Chrome

mweb-chrome-2fa-error-persists.mov

mweb-chrome-code-page-scroll-view.mov

Mobile Web - Safari

mweb-safari-2fa-error-persists.mov

web-code-page-scroll-view.mov

Desktop

desktop-2fa-error-persists.mov

desktop-code-page-scroll-view.mov

iOS

ios-2fa-translation-error-persists.mov

Android

android-2fa-error-persists.mov

mananjadhav · 2023-05-26T06:33:51Z

@MonilBhavsar @BeeMargarida Is the Two Factor Screen loading for you on Android and iOS?

On Android it is abruptly crashing for me without any errors.

android-crash.mov

On iOS I get an exception first, and if I dismiss I see the screen.

Also on iOS I am unable to enter the 2fa code.

ios-stickey-keys.mov

MonilBhavsar · 2023-05-26T11:40:16Z

I'm seeing it too 😭
@BeeMargarida or @thiagobrez if you could please take a look

thiagobrez · 2023-05-29T07:42:07Z

@MonilBhavsar Looking into it now!

thiagobrez · 2023-05-29T08:10:42Z

@mananjadhav @MonilBhavsar

On Android it is abruptly crashing for me without any errors.

On iOS I get an exception first, and if I dismiss I see the screen.

These two ⬆️ were caused by a little issue, incorrectly setting a style. I'm contacting Ana to get permissions to push to her branch.

Also on iOS I am unable to enter the 2fa code.

This one is a known bug, unrelated to 2-FA. Happens only on iOS 16.4, on simulator, in dev mode. See the related issue

mananjadhav

@MonilBhavsar I've finished the checklist. The android tests work fine now.

I've bypassed the iOS issue by pasting the code, instead of typing in.

MonilBhavsar · 2023-05-29T16:09:16Z

@thiagobrez I see the view being disrupted bug on samsung galaxy fold. I think we still need to fix it. Could you please take look, thanks!

mananjadhav · 2023-05-29T16:25:00Z

Ohh yeah 🤦. Thanks for catching @MonilBhavsar

thiagobrez · 2023-05-30T08:36:43Z

Great catch @MonilBhavsar, thanks! I didn't know it was supported, I'll keep an eye next time 🙇🏻

I added a new breakpoint for extra small screens like this. I'm using < 320 since this is the smallest preset breakpoint on Chrome. Let me know what you think :)

MonilBhavsar · 2023-05-31T07:02:25Z

Thanks for fixing this! The view looks good now, but looks like user can't see continue button. I wonder should we use ScrollView in codespage

thiagobrez · 2023-05-31T08:41:54Z

Thanks @MonilBhavsar! Fixed ✅

MonilBhavsar · 2023-05-31T09:11:06Z

ios/Podfile.lock

Thanks! Is this file really needed?

I just merged main and installed dependencies. I guess in some other PR the dependencies were updated but the lockfile wasn't pushed

MonilBhavsar · 2023-05-31T10:43:22Z

Thanks! @mananjadhav could you please help to review and test latest scroll view change. Thanks!

mananjadhav

Just tested the CodePages ScrollView.

MonilBhavsar · 2023-06-01T09:56:37Z

src/styles/styles.js

-        borderRadius: 16,
-        marginTop: 32,
+    twoFactorAuthCodesBox: ({isExtraSmallScreenWidth, isSmallScreenWidth}) => {
+        let paddingHorizontal = styles.ph15;


@shawnborton buddy check please for these new style

What exactly am I reviewing here? Just the screenshots from the first comment?

sorry, Nothing has changed much in the view. I wanted to get an eye on padding prooerty we added to support view for extra small devices.

Ah got it. Can you show me screenshots of what this looks like in practice please? Thanks!

@shawnborton Here you go:

Desktop, using padding from Figma: `styles.ph15`

Small screen width (<= 800px), decreased padding until it doesn't break: `styles.ph10`

Extra small screen width (<= 320px), decreased padding until it doesn't break: `styles.ph4`

Okay cool, that works for me!

Thanks shawn!

MonilBhavsar

Thanks! Looks good to me

MonilBhavsar

Thanks! Looks good to me

OSBotify · 2023-06-02T22:35:57Z

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.23-3 🚀

platform	result
🤖 android 🤖	cancelled 🔪
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	cancelled 🔪

OSBotify · 2023-06-05T19:05:14Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.23-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-06-05T19:29:50Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.23-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

cristipaval · 2023-06-05T22:20:48Z

Heads up, this PR added this regression

s77rt · 2023-06-26T14:30:43Z

src/pages/ValidateLoginPage/index.website.js

+                {currentAuthState === CONST.AUTO_AUTH_STATE.FAILED && <ExpiredValidateCodeModal />}
+                {currentAuthState === CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN && is2FARequired && !isSignedIn && <TfaRequiredModal />}
+                {currentAuthState === CONST.AUTO_AUTH_STATE.NOT_STARTED && isSignedIn && <AbracadabraModal />}
+                {currentAuthState === CONST.AUTO_AUTH_STATE.NOT_STARTED && !isSignedIn && (


@BeeMargarida Do you remember why we had to add the !isSignedIn condition here? This caused a regression #21258

@s77rt Yes, it came from this commit to fix this bug mentioned in this slack thread.

Before, it was necessary because the check for AbracadabraModal (now JustSignedInModal) used NOT_STARTED state (this is because the state was cleaned on startup - mentioned in the proposal), so we needed to differentiate between that one and the one for ValidateCodeModal. At the time, the only difference was that for the abracadabra, the isSignedIn was true and false for the ValidateCodeModal.
I don't think it was a regression from here specifically, since this changed a lot after this PR.

@BeeMargarida Thank you!

Santhosh-Sellavel · 2023-06-28T13:08:39Z

src/pages/settings/Security/TwoFactorAuth/CodesPage.js

+                    <FixedFooter style={[styles.twoFactorAuthFooter]}>
+                        <Button
+                            success
+                            text={props.translate('common.next')}
+                            onPress={() => Navigation.navigate(ROUTES.SETTINGS_2FA_VERIFY)}
+                            isDisabled={isNextButtonDisabled}
+                        />
+                    </FixedFooter>


Seems we accidentally included FixedFooter inside scroll containers and that led to this regression #20138

BeeMargarida added 9 commits May 19, 2023 11:29

fix: change all 'two factor' to 'two-factor' in translation

bf1cc35

fix: make two-factor consistent in code comments as well

9a8ec11

fix: use download translation in CodesPage

6a3ec66

fix: wrong state in validate login modal after 2FA

96e49c3

refactor: rename isTfaRequired to is2FARequired

93936a2

fix: broken view of codes page on small screen

eaf3059

fix: clear account error messages when entering verify page

d92dabc

fix: add missing condition for validate login page

4da2f31

style: prettier

d205df2

fabioh8010 reviewed May 19, 2023

View reviewed changes

src/pages/ValidateLoginPage/index.website.js Outdated Show resolved Hide resolved

thiagobrez reviewed May 19, 2023

View reviewed changes

src/pages/settings/Security/TwoFactorAuth/CodesPage.js Outdated Show resolved Hide resolved

BeeMargarida added 2 commits May 19, 2023 14:08

refactor: extract state to variable

bd39be1

refactor: move value to styles object

16a9656

thiagobrez reviewed May 19, 2023

View reviewed changes

src/styles/utilities/spacing.js Outdated Show resolved Hide resolved

fix: wrong padding horizontal variable

c7e295f

BeeMargarida marked this pull request as ready for review May 19, 2023 13:23

BeeMargarida requested a review from a team as a code owner May 19, 2023 13:23

melvin-bot bot requested review from mananjadhav and MonilBhavsar and removed request for a team May 19, 2023 13:23

MonilBhavsar reviewed May 22, 2023

View reviewed changes

mananjadhav previously approved these changes May 24, 2023

View reviewed changes

fix: incorrectly setting style

7da13b0

thiagobrez dismissed mananjadhav’s stale review via 7da13b0 May 29, 2023 08:34

mananjadhav previously approved these changes May 29, 2023

View reviewed changes

fix: padding in extra narrow screens

1ee87e7

thiagobrez dismissed mananjadhav’s stale review via 1ee87e7 May 30, 2023 08:31

chore: fix conflicts

931c335

thiagobrez added 2 commits May 31, 2023 09:35

chore: add scrollview to CodesPage

48a90a9

Merge branch 'main' into fix/19187-2fa_fixes

023e5d9

MonilBhavsar reviewed May 31, 2023

View reviewed changes

mananjadhav approved these changes May 31, 2023

View reviewed changes

MonilBhavsar reviewed Jun 1, 2023

View reviewed changes

MonilBhavsar approved these changes Jun 1, 2023

View reviewed changes

MonilBhavsar merged commit 499d606 into Expensify:main Jun 2, 2023

OSBotify mentioned this pull request Jun 2, 2023

Deploy Checklist: New Expensify 2023-06-02 #20042

Closed

32 tasks

kbecciv mentioned this pull request Jun 3, 2023

[HOLD for payment 2023-06-19] [$1000] Web - Sign in - Blank screen instead of 'Abracadabra, you are signed in!' for 2FA account #20136

Closed

6 tasks

melvin-bot bot mentioned this pull request Jun 5, 2023

[HOLD for payment 2023-06-12] [Tracking Issue]Bug fixes: 2FA #19187

Closed

MonilBhavsar mentioned this pull request Jun 9, 2023

[HOLD for payment 2023-06-29] [$1000] iOS - Copy and Download buttons overlap with Next button when app is in Spanish #20138

Closed

6 tasks

s77rt reviewed Jun 26, 2023

View reviewed changes

s77rt mentioned this pull request Jun 26, 2023

[HOLD for payment 2023-07-06] [$1000] Login - Page freezes when navigating to secondary login verification link #21258

Closed

6 tasks

Santhosh-Sellavel reviewed Jun 28, 2023

View reviewed changes

s77rt mentioned this pull request Jun 28, 2023

fix style copy button #21698

Merged

57 tasks

fix: 2FA bug fixes #19260

fix: 2FA bug fixes #19260

Conversation

BeeMargarida commented May 19, 2023

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 19, 2023

MonilBhavsar left a comment

Choose a reason for hiding this comment

mananjadhav left a comment

Choose a reason for hiding this comment

mananjadhav commented May 26, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

mananjadhav commented May 26, 2023

MonilBhavsar commented May 26, 2023

thiagobrez commented May 29, 2023

thiagobrez commented May 29, 2023

mananjadhav left a comment • edited Loading

Choose a reason for hiding this comment

MonilBhavsar commented May 29, 2023

mananjadhav commented May 29, 2023

thiagobrez commented May 30, 2023

MonilBhavsar commented May 31, 2023

thiagobrez commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonilBhavsar commented May 31, 2023

mananjadhav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagobrez Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonilBhavsar left a comment

Choose a reason for hiding this comment

MonilBhavsar left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 2, 2023

OSBotify commented Jun 5, 2023

OSBotify commented Jun 5, 2023

cristipaval commented Jun 5, 2023

Choose a reason for hiding this comment

BeeMargarida Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

mananjadhav commented May 26, 2023 •

edited

Loading

mananjadhav left a comment •

edited

Loading

thiagobrez Jun 2, 2023 •

edited

Loading

BeeMargarida Jun 26, 2023 •

edited

Loading

Santhosh-Sellavel Jun 28, 2023 •

edited

Loading